add user and password to uri if the uri does not contain user password #560

naughtyGitCat · 2022-09-13T06:47:24Z

[[feature]split user and password from uri #559] Links to other linked pull requests (optional).

Tests passed.
Fix conflicts with target branch.
Update jira ticket description if needed.
Attach screenshots/console output to confirm new behavior to jira ticket, if applicable.

Once all checks pass and the code is ready for review, please add pmm-review-exporters team as the reviewer. That would assign people from the review team automatically. Report any issues on our Forum or Discord.

it-percona-cla · 2022-09-13T06:47:28Z

All committers have signed the CLA.

main.go

naughtyGitCat

improve compatibility, make the change do impact less

tshcherban · 2022-10-07T12:45:43Z

@ShashankSinha252 passing both CI (FeatureBuild) and metrics test

naughtyGitCat · 2022-10-11T09:03:55Z

@ShashankSinha252 are there any more changes that should be made by me?

Aleksa-del · 2022-11-22T12:30:34Z

Hello, @naughtyGitCat! Thank you for your contribution! We would love to send you some gifts. Please, contact us at [email protected] with your mailing address, phone number (for delivery only) and T-shirt size.

BupycHuk

Hi @naughtyGitCat can I ask what you mean by exposing URL in the original issue? In case we expose the full URL in logs it's a problem, but the solution you made probably won't help, because they're still in the URL variable. So it would be good to understand how do you expose URL?

BupycHuk · 2022-11-30T10:49:34Z

exporter/exporter.go

+		e.opts.EnableDiagnosticData = false
+		e.opts.EnableDBStats = false
+		e.opts.EnableCollStats = false
+		e.opts.EnableTopMetrics = false
+		e.opts.EnableReplicasetStatus = false
+		e.opts.EnableIndexStats = false


Is it related to the issue we are solving in this PR?
it would be good to write some logs here to show that we are disabling collectors.

it is a separate issue.
if we run the exporter to monitoring arbiter role instance. the exporter will be hung to stuck.
It seems I should make a separate issue and pr. if possible, i will learn how to separate commits from pr before this week ends

Great, thank you

Hi @naughtyGitCat can I ask what you mean by exposing URL in the original issue? In case we expose the full URL in logs it's a problem, but the solution you made probably won't help, because they're still in the URL variable. So it would be good to understand how do you expose URL?

my purpose is to try to expose the monitored instance url, the benefit is easily identifying the exporter instance target DB instance. a host maybe runs multi exporter instances to monitor multi different DB instances. currently, the exporter can not easily identify which DB it belongs to.

by adding url, we can safely expose the url info, and hide the user pass info by the env value

Do you want to expose URL in mongodb_exporter logs/metrics or somehow else?

Do you want to expose URL in mongodb_exporter logs/metrics or somehow else?

I want to expose the url in cmdline, which can be easily identified by ps command, the log or metric need more works to fetch url info. hard to manipulate exporter instance

aha, ok, it makes sense

BupycHuk · 2022-11-30T10:52:17Z

main.go

-		log.Debugf("Prepending mongodb:// to the URI")
+	// IF user@pass not contained in uri AND custom user and pass supplied in arguments
+	// DO concat a new uri with user and pass arguments value
+	if !strings.Contains(opts.URI, "@") && opts.User != "" && opts.Password != "" {


it should be two separate ifs, in case we have data source value like username:password@host:port without mongodb:// prefix this if will be skipped.

in my modification, I removed possible exits mongodb:// prefix. and after the user password re parse logic, the prefix will be added back when this if ends.

and, in username:password@host:port without mongodb:// circumstances, this if(aka my modifcation) will not be triggered.

yes, and it changes current behavior and breaks backward compatibility, because before your changes we were prepending mongodb://

yes, U are right, I will change it to achieve backward compatibility

I have taken a look on changes carefully,
in this pr,
I add two new arguments user and password.

for backward compatibility, the two new user and password arguments will be an empty string when not supplied, this if clause will not be triggered.
only when using new user and password argument is supplied and there is no user and pass in origin URI. this if will be triggered.

besides, if the user and password are both supplied in the origin URI and arguments(env or --user --pass), the exporter will take the URI one. (if not triggered )
the user, pass value in URI will have more priority than arguments

Can we extract whole this logic and write a test so it will be clear what's wrong

the case which was working and won't work. username:password@host:port should be converted to mongodb://username:password@host:port

haven extracted logic, and write a test. 1cdf1a6

but the check is stucked by

ok github.com/percona/mongodb_exporter/internal/tu 0.097s ? github.com/percona/mongodb_exporter/internal/util [no test files] FAIL make: *** [Makefile:104: test-race] Error 1

This error seems unrelated to the code I've submitted.

naughtyGitCat · 2022-12-12T07:00:00Z

I have split two arbiter-related commits to #607

and this pr will only contains user pass expose commits

BupycHuk · 2023-09-01T08:37:18Z

main_test.go

+	t.Logf("Origin: %s", originalBareURI)
+	t.Logf("Expect: %s", originalBareURI)
+	t.Logf("Result: %s", newUri)
+	if newUri != originalBareURI {


Suggested change

if newUri != originalBareURI {

if newUri != originalPrefixBareURI {

This is the expected behavior.

i have reorgnized the test code, 24987e4

could you review it again, thanks

BupycHuk · 2023-09-01T10:05:31Z

main_test.go

+			origin:      "127.0.0.1",
+			newUser:     "",
+			newPassword: "",
+			expect:      "127.0.0.1",


the expectation here is mongodb://127.0.0.1, that's how it used to work

Suggested change

expect: "127.0.0.1",

expect: "mongodb://127.0.0.1",

my mistake, have correct it both in main.go and main_test.go

it does need a mongod:// prefix in all situation, according to https://www.mongodb.com/docs/manual/reference/connection-string/

BupycHuk · 2023-09-01T10:20:10Z

main.go

+		// add back mongodb://
+		uri = "mongodb://" + uri


let's remove it, anyway we do it a few lines later

BupycHuk · 2023-09-01T10:21:04Z

please fix linters

naughtyGitCat · 2023-09-01T10:27:08Z

@BupycHuk seems weird, the below check

ok  	github.com/percona/mongodb_exporter/internal/tu	(cached)
?   	github.com/percona/mongodb_exporter/internal/util

have no relation with my change, and some of the checks seem to randomly pass and failed

and the check function InspectContainer seems related to docker env only other than the main logic code

BupycHuk · 2023-09-01T10:37:22Z

I meant this one https://github.com/percona/mongodb_exporter/actions/runs/6048290809/job/16413363823?pr=560

BupycHuk · 2023-09-01T10:37:40Z

Yeah, tests are unstable lately and we are working on it

naughtyGitCat · 2023-09-01T10:43:24Z

I meant this one https://github.com/percona/mongodb_exporter/actions/runs/6048290809/job/16413363823?pr=560

failed with:
diff --git a/main_test.go b/main_test.go
is caused by
git diff --exit-code
and this command only means to mark two commits changed or not.
I think it was related to the merging from percona main to forked main

naughtyGitCat · 2023-09-01T10:45:16Z

ci and lint check rules look like teetering

BupycHuk · 2023-09-01T13:36:56Z

main_test.go

@@ -16,6 +16,7 @@
 package main

 import (
+	"github.com/stretchr/testify/assert"


this import should be moved below as a separate block

I guess you mean code orders like this

import ( "context" "fmt" "strings" "testing" "time" "github.com/prometheus/client_golang/prometheus/testutil" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "go.mongodb.org/mongo-driver/bson" "github.com/percona/mongodb_exporter/internal/tu" )

changed, years ago I did the same order like this, but when the Goland and gofmt auto changed dependency order, I thought the auto reordered was more go-style.

naughtyGitCat requested a review from a team as a code owner September 13, 2022 06:47

naughtyGitCat requested review from ShashankSinha252 and tshcherban and removed request for a team September 13, 2022 06:47

denisok added the community label Sep 13, 2022

denisok added this to the v0.35.0 milestone Sep 14, 2022

denisok linked an issue Sep 14, 2022 that may be closed by this pull request

[feature]split user and password from uri #559

Closed

This was referenced Sep 14, 2022

Mongo URI password visible in process list on Linux #561

Open

Container: Passwort file as source of the mongodb passwort #380

Open

ShashankSinha252 suggested changes Sep 29, 2022

View reviewed changes

main.go Outdated Show resolved Hide resolved

naughtyGitCat commented Sep 30, 2022

View reviewed changes

tshcherban approved these changes Oct 7, 2022

View reviewed changes

artemgavrilov requested a review from ShashankSinha252 November 17, 2022 17:15

artemgavrilov assigned ShashankSinha252 Nov 17, 2022

artemgavrilov requested a review from BupycHuk November 30, 2022 10:34

artemgavrilov assigned BupycHuk and unassigned ShashankSinha252 Nov 30, 2022

BupycHuk requested changes Nov 30, 2022

View reviewed changes

naughtyGitCat mentioned this pull request Nov 30, 2022

fail to run exporter on mongodb arbiter instance #598

Closed

naughtyGitCat closed this Dec 12, 2022

naughtyGitCat force-pushed the main branch from a143ff8 to 804443c Compare December 12, 2022 06:55

naughtyGitCat added 2 commits December 12, 2022 14:55

add user and password to uri if the uri does not contain user password

5419d8e

improve compatibility

b793c84

naughtyGitCat reopened this Dec 12, 2022

naughtyGitCat removed the request for review from ShashankSinha252 January 10, 2023 09:25

naughtyGitCat requested a review from BupycHuk January 10, 2023 09:25

naughtyGitCat added 4 commits September 1, 2023 12:40

extract whole buildURI logic and write a test

1cdf1a6

Merge branch 'main' into main

d2fb77d

add description for MONGODB_USER and MONGODB_PASSWORD in README.md

36b3fa7

fix for lint check

6c101da

BupycHuk reviewed Sep 1, 2023

View reviewed changes

naughtyGitCat added 2 commits September 1, 2023 16:44

make duplications test code simplified

c9cddd9

change properties order to make read easier

24987e4

BupycHuk reviewed Sep 1, 2023

View reviewed changes

add back mongodb:// append logic

1b44f72

BupycHuk approved these changes Sep 1, 2023

View reviewed changes

clean duplicate mongodb:// append logic

7803f17

try to fix lint

94de219

BupycHuk reviewed Sep 1, 2023

View reviewed changes

move third party dependency below to internal dependency

bd3922c

naughtyGitCat requested review from ShashankSinha252 and tshcherban September 8, 2023 03:00

BupycHuk merged commit 9571d21 into percona:main Sep 13, 2023
3 of 8 checks passed

undefined-moe mentioned this pull request Dec 18, 2023

prometheus-mongodb-exporter: 0.39.0 -> 0.40.0 NixOS/nixpkgs#275108

Merged

	if newUri != originalBareURI {
	if newUri != originalPrefixBareURI {

add user and password to uri if the uri does not contain user password #560

add user and password to uri if the uri does not contain user password #560

Conversation

naughtyGitCat commented Sep 13, 2022

it-percona-cla commented Sep 13, 2022 • edited Loading

naughtyGitCat left a comment

Choose a reason for hiding this comment

tshcherban commented Oct 7, 2022

naughtyGitCat commented Oct 11, 2022

Aleksa-del commented Nov 22, 2022

BupycHuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naughtyGitCat Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naughtyGitCat Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naughtyGitCat commented Dec 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BupycHuk commented Sep 1, 2023

naughtyGitCat commented Sep 1, 2023 • edited Loading

BupycHuk commented Sep 1, 2023

BupycHuk commented Sep 1, 2023

naughtyGitCat commented Sep 1, 2023

naughtyGitCat commented Sep 1, 2023

Choose a reason for hiding this comment

naughtyGitCat Sep 2, 2023 • edited Loading

Choose a reason for hiding this comment

it-percona-cla commented Sep 13, 2022 •

edited

Loading

naughtyGitCat Nov 30, 2022 •

edited

Loading

naughtyGitCat Sep 1, 2023 •

edited

Loading

naughtyGitCat commented Sep 1, 2023 •

edited

Loading

naughtyGitCat Sep 2, 2023 •

edited

Loading